Skip to content

Go ethereum version upgrade to Punisher (v1.8.27)#901

Merged
pdyraga merged 7 commits into
masterfrom
go-ethereum-version-upgrade
Jul 8, 2019
Merged

Go ethereum version upgrade to Punisher (v1.8.27)#901
pdyraga merged 7 commits into
masterfrom
go-ethereum-version-upgrade

Conversation

@dimpar
Copy link
Copy Markdown
Contributor

@dimpar dimpar commented Jul 4, 2019 •
edited
Loading

Closes #741

In this PR we are upgrading go-ethereum from Budapest (v1.8.16) released in 24 Sep 2018 to Punisher (v1.8.27) released in 17 Apr 2019

dimpar added 3 commits July 4, 2019 15:20
@dimpar
Changing import of crypto/sha3
6625693 
- Import crypto/sha3 from `golang.org/x` instead of
`github.com/ethereum/go-ethereum`. This change was introduced in
Byzantium v1.8.21 PR: ethereum/go-ethereum#18390
For our upgrade we also need to point to golang.org/x/crypto/sha3 so the
dependencies won't break.
@dimpar
Fixing a couple of tests that a new version of go-ethereum broke
2ff040c 
- Fixed errors in error_resolver_test.go due to a newer version of
abi.go::MethodById() introduced in commit: 96fd50be10885c9b3033404df698177fdb63d036
@dimpar
Upgrading go-ethereum from Budapest v1.8.16 to Punisher v1.8.27
9f38842
@dimpar
Copy link
Copy Markdown
Contributor Author

dimpar commented Jul 4, 2019 •
edited
Loading

During the update a couple of things broke:

  1. due to vendor, crypto, swarm: switch over to upstream sha3 package ethereum/go-ethereum#18390 in Byzantium Revert (v1.8.21), we have to import "golang.org/x/crypto/sha3" instead of "github.com/ethereum/go-ethereum/crypto/sha3"
  2. These tests were failing because they changed abi.go::MethodById()in commit: 96fd50be10885c9b3033404df698177fdb63d036
--- FAIL: TestErrorResolverHandlesUnknownMethodResponses (0.00s)
    error_resolver_test.go:130:
        expected: {error containing ["no method with id"]}
        actual:   got [data too short ( 11ytes) for abi method lookup] while resolving original error [OG] on return [[0 0 0 1]]
--- FAIL: TestErrorResolverHandlesBadParameterResponses (0.00s)
    error_resolver_test.go:145:
        expected: {error containing ["length insufficient"]}
        actual:   got [data too short ( 11ytes) for abi method lookup] while resolving original error [OG] on return [[8 195 121 160]]
    error_resolver_test.go:158:
        expected: {error containing ["would go over slice boundary"]}
        actual:   got [data too short ( 11ytes) for abi method lookup] while resolving original error [OG] on return [[8 195 121 160 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1]]
    error_resolver_test.go:173:
        expected: {error containing ["length insufficient"]}
        actual:   got [data too short ( 11ytes) for abi method lookup] while resolving original error [OG] on return [[8 195 121 160 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 32 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1]]
--- FAIL: TestErrorResolverHandlesGoodErrorResponse (0.00s)
    error_resolver_test.go:194:
        expected: {error containing ["[]"]}
        actual:   got [data too short ( 11ytes) for abi method lookup] while resolving original error [OG] on return [[8 195 121 160 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 32 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]]
    error_resolver_test.go:211:
        expected: {error containing ["Something's gone awry."]}
        actual:   got [data too short ( 11ytes) for abi method lookup] while resolving original error [OG] on return [[8 195 121 160 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 32 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 22 83 111 109 101 116 104 105 110 103 39 115 32 103 111 110 101 32 97 119 114 121 46]]
FAIL
FAIL  github.com/keep-network/keep-core/pkg/chain/ethereum/ethutil  2.041s

@dimpar
Copy link
Copy Markdown
Contributor Author

dimpar commented Jul 4, 2019

Tested the following:

  • truffle exec ./scripts/genesis.js --network local - request looked good and a new group was registered on chain.

  • truffle exec ./scripts/request-relay-entry.js --network local - request looked good and a new group was registered on chain.

Successfully requested relay entry with RequestId = 2

---Transaction Summary---
From:0x20b425f2cbb6683be184e821f6d3dcbe3905527d
To:0x278fe2d25d8c652aa33b7a10a92425ff531fd045
BlockNumber:48239
TotalGas:157569
TransactionHash:0x50a067f334136da9b83240952d0b2e11f3415898b8342c41d69c8a64996f2547

@pdyraga
Copy link
Copy Markdown
Member

pdyraga commented Jul 5, 2019

I think this card should Closes the referenced issue. Why do we Refs here?

pdyraga
pdyraga reviewed Jul 5, 2019
View reviewed changes
Comment thread pkg/chain/ethereum/ethutil/error_resolver.go Outdated
pdyraga
pdyraga reviewed Jul 5, 2019
View reviewed changes
Comment thread pkg/chain/ethereum/ethutil/error_resolver_test.go Outdated
@pdyraga
Copy link
Copy Markdown
Member

pdyraga commented Jul 5, 2019

It may be worth to add require(false, "my revert msg") to e.g. relayEntry contract function, run genesis and see if error resolver works fine. I know unit tests are passing but geth surprised us a lot of times.

@pdyraga pdyraga requested a review from Shadowfiend July 5, 2019 06:04
@dimpar
Copy link
Copy Markdown
Contributor Author

dimpar commented Jul 5, 2019 •
edited
Loading

when adding require(false, "my revert msg") into the relayEntry and requesting genesis, the following error pops:

Genesis entry submission failed with { Error: Transaction: 0xfcd1b4e8cb16524361f718a3be69d5607fab68750d31f081e75583976eb44120 exited with an error (status 0).
     Please check that the transaction:
     - satisfies all conditions set by Solidity `require` statements.
     - does not trigger a Solidity `revert` statement.
[...]

seems like we can't just trigger require(false,...) manually against geth.
Any other idea or settle on tests that we have? I mean they still run on chain.

@pdyraga
Copy link
Copy Markdown
Member

pdyraga commented Jul 5, 2019 •
edited
Loading

Oh, right - genesis submits relay entry from truffle script so we'll not trigger error resolver 🤦‍♂
However, we can add require(_signingId== 0, "hahahaha") and this will fail every other entry but genesis. So we can genesis - it should work fine and then request new entry - submission should fail.

@Shadowfiend
Copy link
Copy Markdown
Contributor

Shadowfiend commented Jul 8, 2019

data too short ( 11ytes)

Well that's sloppy heh.

dimpar added 3 commits July 8, 2019 17:53
@dimpar
Fixing error_resolver_test.go
abf8e51 
- Reverted added 1 byte at the end of a method ABI signature.
- Allocated 4 bytes instead of 3 for errorID construction.
@dimpar
Adding an explanation for parametes in &fixedReturnCaller{}
28c5b03
@dimpar
Adding Gopkg.lock after go-ethereum upgrade
b211cd4
@pdyraga
Copy link
Copy Markdown
Member

pdyraga commented Jul 8, 2019 •
edited
Loading

Testing... 👓

@pdyraga
Copy link
Copy Markdown
Member

pdyraga commented Jul 8, 2019

Dropped my vendor, downloaded all dependencies, regenerated code, rebuilt everything, tested genesis & relay request - works as expected.

@pdyraga
Copy link
Copy Markdown
Member

pdyraga commented Jul 8, 2019

All @Shadowfiend comments were addressed as requested, so I am merging it.

@pdyraga pdyraga merged commit 52d1283 into master Jul 8, 2019
@pdyraga pdyraga deleted the go-ethereum-version-upgrade branch July 8, 2019 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Shadowfiend Shadowfiend Awaiting requested review from Shadowfiend

1 more reviewer

@pdyraga pdyraga pdyraga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

go-ethereum version upgrade

3 participants

@dimpar @pdyraga @Shadowfiend